[Bug Fix] Grouping Cluster Resources by group and Kind #875

arunikadhal · 2022-02-25T15:34:41Z

What are you trying to accomplish with this PR?
Fixes #873

How is this accomplished?
Instead of grouping by kind in DeployTask#discover_resources, we now group by group and kind to avoid picking the wrong CRD when there are duplicates of the same kind.

What could go wrong?
...

lib/krane/kubernetes_resource/custom_resource_definition.rb

peiranliushop · 2022-02-25T15:46:00Z

lib/krane/deploy_task.rb

@@ -238,9 +238,10 @@ def secrets_from_ejson
    def discover_resources
      @logger.info("Discovering resources:")
      resources = []
-      crds_by_kind = cluster_resource_discoverer.crds.group_by(&:kind)
+      crds_by_group_kind = cluster_resource_discoverer.crds.group_by(&:group_kind)


Are there tests to cover this change?

will be great to test this change, this is supposed to change the order of the items

added new tests to cover these changes

this is supposed to change the order of the items

I don't quite understand what you mean by this, could you explain please?

yeah, sorry, I misread the issue in #873 and I thought a consequence of this PR is that it'll change the order of the items in some list, however, this change is about grouping differently. Sorry for the noise

d1egoaz · 2022-02-25T17:06:06Z

it seems some tests are broken:

Error:
KraneDeployTest#test_raise_on_yaml_missing_kind:
TypeError: no implicit conversion of nil into String

d1egoaz · 2022-03-04T16:51:51Z

lib/krane/deploy_task.rb

      @template_sets.with_resource_definitions(current_sha: @current_sha, bindings: @bindings) do |r_def|
-        crd = crds_by_kind[r_def["kind"]]&.first
+        grouping, version = r_def.dig("apiVersion").split("/")
+        group = version ? grouping : "core"


what does mean "core" in this context?

When extracting group, lib/krane/kubernetes_resource.rb returns the group defined in the resource definition or core. We need to do something similar here for r_def but we cannot use the same method in lib/krane/kubernetes_resource.rb so I decided to replicate the behaviour here instead (hence the core).

lib/krane/deploy_task.rb

* Fixes

timothysmith0609 · 2022-03-10T19:30:52Z

lib/krane/deploy_task.rb

@@ -238,9 +238,10 @@ def secrets_from_ejson
    def discover_resources
      @logger.info("Discovering resources:")
      resources = []
-      crds_by_kind = cluster_resource_discoverer.crds.group_by(&:kind)
+      crds_by_group_kind = cluster_resource_discoverer.crds.group_by(&:group_kind)


this is supposed to change the order of the items

I don't quite understand what you mean by this, could you explain please?

timothysmith0609 · 2022-03-10T19:36:03Z

lib/krane/kubernetes_resource/custom_resource.rb

@@ -59,7 +59,7 @@ def status
    end

    def type
-      kind
+      group_kind


For other reviewers:

#type is already a somewhat overloaded method. In particular, KubernetesResource#type has as body:

def type @type || self.class.kind end

#type is mainly used internally, most critically in DeployTask#deploy_has_priority_resources?, though it's value also appears in the tool's output.

#type is also aliased to KubernetesResource#kubectl_resource_type, which is used by ResourceCache. There is already a case where #type is overridden to include the group suffix.

All this to say, I'm hoping the use of group_kind here isn't too controversial :)

timothysmith0609 · 2022-03-10T19:37:22Z

lib/krane/kubernetes_resource/custom_resource_definition.rb

+    def group_kind
+      "#{kind}.#{group}"
+    end
+


The behaviour of CustomResourceDefinition#kind and #group is a little strange (you might expect it to return CRD and apiextensions, respectively), but since this is previous behaviour I think it's ok to leverage it

timothysmith0609 · 2022-03-10T19:38:08Z

test/unit/krane/kubernetes_resource/custom_resource_definition_test.rb

-      definition: { "kind" => "UnitTest", "metadata" => { "name" => "test" } })
+      definition: {
+        "kind" => "UnitTest",
+        "apiVersion" => "stable.example.io/v1",


We need to add apiVersion to make these tests pass (since we now parse group info). Since all k8s objects require this field, it makes sense to add it instead of accommodating its absence

timothysmith0609 · 2022-03-10T19:39:00Z

test/integration-serial/serial_deploy_test.rb

@@ -450,7 +487,7 @@ def test_deploying_crs_with_invalid_crd_conditions_fails
      end
    end
    assert_deploy_failure(result)
-    prefixed_name = add_unique_prefix_for_test("Customized-with-customized-params")
+    prefixed_name = add_unique_prefix_for_test('Customized.stable.example.io-with-customized-params').to_s


This is the most visible consequence of our change, and I would argue it's a feature: CRDs are now logged showing both their group and kind. This should make things easier to understand in the case of multiple kinds among difference groups

timothysmith0609 · 2022-03-10T19:45:08Z

You'll also need to fix the CR logic in global_deploy_task, as well (and add test)

arunikadhal · 2022-03-11T20:35:56Z

You'll also need to fix the CR logic in global_deploy_task, as well (and add test)

There seems to be a different issue in global_deploy_task, I have opened a new issue to fix the kind & name discrepancy here.

timothysmith0609 · 2023-04-11T14:01:13Z

Closing as stale

Grouping Cluster Resources by group and Kind

4e123fc

arunikadhal requested a review from a team as a code owner February 25, 2022 15:34

arunikadhal requested review from d1egoaz and peiranliushop and removed request for a team February 25, 2022 15:34

peiranliushop reviewed Feb 25, 2022

View reviewed changes

lib/krane/kubernetes_resource/custom_resource_definition.rb Outdated Show resolved Hide resolved

peiranliushop reviewed Feb 25, 2022

View reviewed changes

Arunika Dhal added 2 commits March 3, 2022 11:49

fixing failing tests

699e170

re-using kind

c614da0

d1egoaz reviewed Mar 4, 2022

View reviewed changes

lib/krane/deploy_task.rb Outdated Show resolved Hide resolved

Arunika Dhal and others added 4 commits March 8, 2022 09:54

removing if condition

d2eba74

begin test

4063283

better comment

be52459

Cr group kind fixes (#879)

2c7fafd

* Fixes

timothysmith0609 reviewed Mar 10, 2022

View reviewed changes

arunikadhal requested review from d1egoaz, peiranliushop and timothysmith0609 March 14, 2022 18:34

timothysmith0609 closed this Apr 11, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Bug Fix] Grouping Cluster Resources by group and Kind #875

[Bug Fix] Grouping Cluster Resources by group and Kind #875

arunikadhal commented Feb 25, 2022

peiranliushop Feb 25, 2022

d1egoaz Mar 4, 2022

arunikadhal Mar 10, 2022

timothysmith0609 Mar 10, 2022

d1egoaz Mar 10, 2022

d1egoaz commented Feb 25, 2022

d1egoaz Mar 4, 2022

arunikadhal Mar 8, 2022

timothysmith0609 Mar 10, 2022

timothysmith0609 Mar 10, 2022 •

edited

Loading

timothysmith0609 Mar 10, 2022

timothysmith0609 Mar 10, 2022

timothysmith0609 Mar 10, 2022

timothysmith0609 commented Mar 10, 2022 •

edited

Loading

arunikadhal commented Mar 11, 2022 •

edited

Loading

timothysmith0609 commented Apr 11, 2023

[Bug Fix] Grouping Cluster Resources by group and Kind #875

[Bug Fix] Grouping Cluster Resources by group and Kind #875

Conversation

arunikadhal commented Feb 25, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

d1egoaz commented Feb 25, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

timothysmith0609 Mar 10, 2022 • edited Loading

Choose a reason for hiding this comment

For other reviewers:

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

timothysmith0609 commented Mar 10, 2022 • edited Loading

arunikadhal commented Mar 11, 2022 • edited Loading

timothysmith0609 commented Apr 11, 2023

timothysmith0609 Mar 10, 2022 •

edited

Loading

timothysmith0609 commented Mar 10, 2022 •

edited

Loading

arunikadhal commented Mar 11, 2022 •

edited

Loading